Add tests - #2
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new RunEngine teardown fixture relies on a private Bluesky attribute (engine._th) and should be hardened to avoid brittle CI failures across Bluesky versions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a first set of automated unit/integration tests for key simulation components (plans, devices, image analysis, and the energy-alignment agent), and wires them into CI via Pixi-driven GitHub Actions workflows. This also includes small supporting tweaks in the production code to make image handling and typing more robust under test.
Changes:
- Add unit tests covering BMM plans, device signal contracts, detector streaming assets, image-analysis utilities, and agent wiring/evaluation.
- Add an integration round-trip test that writes HDF5 frames, ingests them via a local Tiled app, and evaluates outcomes end-to-end.
- Add pytest configuration + Pixi tasks and introduce CI workflows for unit tests (per-push/PR) and integration tests (scheduled).
File summaries
| File | Description |
|---|---|
| tests/conftest.py | Adds shared pytest fixtures for RunEngine lifecycle and document collection. |
| tests/unit/plans/test_bmm.py | Unit tests for energy-change/scan plans and vendored acquisition behavior. |
| tests/unit/devices/test_devices.py | Unit tests validating device signal sources and read/configuration contracts. |
| tests/unit/devices/test_detectors.py | Unit tests for screen detector acquire logic and StreamResource/StreamDatum emission. |
| tests/unit/analysis/test_image.py | Unit tests for image preprocessing/metrics and Matplotlib interaction helpers. |
| tests/unit/agents/test_energy_alignment.py | Unit tests for energy-alignment evaluator behavior and agent wiring. |
| tests/integration/test_energy_alignment_round_trip.py | Integration test for HDF5 → Tiled → evaluator round-trip. |
| src/tst_sim_tools/analysis/image.py | Normalize inputs to owned finite float arrays; fix centering math in analyze_image. |
| src/tst_sim_tools/agents/energy_alignment.py | Tighten typing for UID and suggestions in evaluator call path. |
| src/tst_sim_tools/startup.py | Add Pyright ignore for an import that may be missing in type-checking environments. |
| pyproject.toml | Add dev deps for tests, pytest config, Pixi test tasks, and an integration marker. |
| .github/workflows/unit-tests.yml | New per-PR/push workflow running unit tests in Pixi dev env. |
| .github/workflows/daily-integration-tests.yml | New scheduled/manual workflow running integration tests in Pixi dev env. |
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
A couple of test/implementation details are unnecessarily coupled to private internals or do redundant catalog access, which can reduce long-term stability.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
src/tst_sim_tools/agents/energy_alignment.py:88
- The evaluator fetches
self._client[uid]twice (once in_poll_for_imagesand again to readmetadata). This adds unnecessary client I/O and can reintroduceKeyError/inconsistencies if the catalog is eventually consistent; reuse the same run object instead.
tests/unit/plans/test_bmm.py:320 - This assertion relies on the private
_cacheattribute of an ophyd-async signal, which is not part of the public API and may change across versions. Usinggetattr(..., None)keeps the test focused on cleanup without coupling it to internal implementation details.
This issue also appears in the following locations of the same file:
- line 335
- line 351
- line 418
tests/unit/plans/test_bmm.py:335
- This assertion relies on the private
_cacheattribute of an ophyd-async signal, which is not part of the public API and may change across versions. Usinggetattr(..., None)keeps the test focused on cleanup without coupling it to internal implementation details.
assert detector._cache is None
tests/unit/plans/test_bmm.py:351
- This assertion relies on the private
_cacheattribute of an ophyd-async signal, which is not part of the public API and may change across versions. Usinggetattr(..., None)keeps the test focused on cleanup without coupling it to internal implementation details.
assert detector._cache is None
tests/unit/plans/test_bmm.py:418
- This assertion relies on the private
_cacheattribute of an ophyd-async signal, which is not part of the public API and may change across versions. Usinggetattr(..., None)keeps the test focused on cleanup without coupling it to internal implementation details.
assert sensor._cache is None
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Assisted-by: oh-my-pi:gpt-5.6-terra
There was a problem hiding this comment.
🟡 Changes recommended
The energy-alignment evaluator’s polling loop can mask real KeyErrors and retry forever, and the README coverage badge is currently hard-coded and will become misleading.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/15 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
tests/conftest.py uses invalid collections.abc.Generator[...] type parameterization (likely import-time failure) and needs a Ruff E402 suppression for the Matplotlib import.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/conftest.py:8
- Ruff/pycodestyle E402 will flag imports after executable statements. Since
MPLBACKENDmust be set before importing Matplotlib, add a targeted# noqa: E402on the Matplotlib import.
tests/conftest.py:21
collections.abc.Generatorrequires three type parameters (yield, send, return).Generator[RunEngine]is invalid and may fail at import time; annotate asGenerator[RunEngine, None, None]for this yielding fixture.
def run_engine() -> Generator[RunEngine]:
- Files reviewed: 14/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The updated energy-alignment evaluator polling can raise KeyError once a run exists but its primary/image data are not yet readable in Tiled, which is likely to cause flakiness in real deployments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/conftest.py:8
- This import comes after executable code (the MPLBACKEND environment set), which will trigger Ruff E402 if the tests are ever linted. If the backend must be set before importing Matplotlib, add a targeted noqa on the import line to document/justify the ordering.
- Files reviewed: 14/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are test/CI-focused with small, well-scoped code tweaks, and the remaining feedback is limited to non-blocking naming/spelling cleanup opportunities.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
tests/integration/test_energy_alignment_round_trip.py:27
EnergyAlignmentEvalutationis misspelled ("Evalutation" vs "Evaluation"). Consider exposing a correctly-spelled alias in the library and switching this integration test to use it, while retaining the existing name for compatibility.
tests/unit/agents/test_energy_alignment.py:26EnergyAlignmentEvalutationis misspelled ("Evalutation" vs "Evaluation"). Adding more call sites in tests makes a future correction harder; consider introducing a correctly-spelled alias (keeping the old name for backward compatibility) and updating tests to use the alias.
tests/unit/devices/test_devices.py:7XRTRectangularAperatureis misspelled ("Aperature" vs "Aperture"). If you intend to fix the public API spelling eventually, consider adding a correctly-spelled alias and using it in tests to avoid propagating the typo.
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Assisted-by: oh-my-pi:gpt-5.6-sol
Adds both unit tests and integration tests for the existing setup.